Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

don't move files in cache twice, fixes renaming for objectstores #17641

Merged
merged 19 commits into from
Oct 19, 2015

Conversation

butonic
Copy link
Member

@butonic butonic commented Jul 14, 2015

@scrutinizer-notifier
Copy link

A new inspection was created.

@butonic
Copy link
Member Author

butonic commented Jul 14, 2015

fixes #15702

@DeepDiver1975
Copy link
Member

hmmm .. behavioral change with no adoption of unit tests? This is fishy - please investigate - THX

@DeepDiver1975
Copy link
Member

👎 until impact/unit testing solved - THX

@bboule
Copy link

bboule commented Jul 17, 2015

For what is is worth I have applied this change in the lab and it works as expected (files and folders no longer disappear on rename)...hope this is helpful!!!

@MorrisJobke
Copy link
Contributor

👎 until impact/unit testing solved - THX

@butonic @icewind1991 Can you have a look at this please

@LorenzoLuconi
Copy link

Patch applied in my owncloud installation using public openstack swift service as storage and now it works. I hope to see this branch merged as soon as possible.

@butonic
Copy link
Member Author

butonic commented Oct 2, 2015

superseded by #19414 which contains this fix as well as a testsuite

@butonic butonic closed this Oct 2, 2015
@butonic butonic deleted the fix_objectstore_rename branch October 2, 2015 08:42
@butonic butonic restored the fix_objectstore_rename branch October 9, 2015 05:53
@butonic butonic reopened this Oct 9, 2015
@butonic
Copy link
Member Author

butonic commented Oct 9, 2015

Reopening because the testsuite is now in place.

@DeepDiver1975 please enable objectstore tests for this one

@butonic
Copy link
Member Author

butonic commented Oct 9, 2015

pinging @icewind1991 and @MorrisJobke for review now that the testsuite is in place.

@DeepDiver1975
Copy link
Member

@DeepDiver1975 please enable objectstore tests for this one

done

@DeepDiver1975 DeepDiver1975 added this to the 8.2-current milestone Oct 9, 2015
@icewind1991
Copy link
Contributor

Change looks good 👍

@DeepDiver1975
Copy link
Member

TODO:

  • ci is hanging for what ever reason
  • I really insist on a unit test which covers the changed behavior

@butonic
Copy link
Member Author

butonic commented Oct 12, 2015

@icewind1991 keep it coming! This is awesome!

@MorrisJobke
Copy link
Contributor

SQLite seems to fail:

11:49:02 1) Test_Files_Sharing_Watcher::testFolderSizePropagationToOwnerStorage
11:49:02 Failed asserting that true is false.
11:49:02 
11:49:02 /ssd/jenkins/workspace/server-master-linux-php7-ci/database/sqlite/label/SLAVE/apps/files_sharing/tests/watcher.php:123
11:49:02 
11:49:02 2) Test_Files_Sharing_Watcher::testSubFolderSizePropagationToOwnerStorage
11:49:02 Failed asserting that true is false.
11:49:02 
11:49:02 /ssd/jenkins/workspace/server-master-linux-php7-ci/database/sqlite/label/SLAVE/apps/files_sharing/tests/watcher.php:153

@DeepDiver1975
Copy link
Member

rebased ...

@DeepDiver1975
Copy link
Member

Nice job @icewind1991 🚀

@DeepDiver1975
Copy link
Member

I'm actually setting the milestone to 9.0 since this pr is against master.

@karlitschek we need to backport this to fix serious issues in 8.2.1

@DeepDiver1975 DeepDiver1975 modified the milestones: 9.0-next, 8.2.1-next-maintenance Oct 17, 2015
@karlitschek
Copy link
Contributor

agreed. please backport

@MorrisJobke
Copy link
Contributor

The unit tests crash on a btrfs docker host ...

Turning ON incompat feature 'mixed-bg': mixed data and metadata block groups
Turning ON incompat feature 'extref': increased hardlink limit per file to 65536
SMALL VOLUME: forcing mixed metadata/data groups

WARNING! - Btrfs v3.12 IS EXPERIMENTAL
WARNING! - see http://btrfs.wiki.kernel.org before using

Created a data/metadata chunk of size 8388608
fs created label (null) on /tmp/osddata
        nodesize 4096 leafsize 4096 sectorsize 4096 size 300.00MiB
Btrfs v3.12
mounting via loopback
mount: Could not find any loop device. Maybe this kernel does not know
       about the loop device? (If so, recompile or `modprobe loop'.)

@butonic Is there a way to fix this easily? Otherwise I'm fine with this but can't test it.

But code wise this looks good 👍

@butonic
Copy link
Member Author

butonic commented Oct 19, 2015

@MorrisJobke hm did you run autotest or just start the container? It needs to run in privileged mode so that loopback devices can be used.

Another solution would be to autodetect btrfs and then not do the whole loobpack device magic as docker can then use the native snapshots to do the fs layering. You could try ripping the loopback stuff from https://github.com/owncloud/core/blob/master/tests/objectstore/entrypoint.sh to see if that helps.

Another thing is that the test currently uses a 300MB storage ... which is enough to run the test suite but might not be enough to test large file uploads.

@MorrisJobke
Copy link
Contributor

@MorrisJobke hm did you run autotest or just start the container?

I did:

$ PRIMARY_STORAGE_CONFIG=swift ./autotest.sh sqlite

I will try to add a condition for btrfs ... but I think this can be hard to detect 🙈

@DeepDiver1975
Copy link
Member

I will try to add a condition for btrfs ... but I think this can be hard to detect 🙈

I see no need in spending more time into this - @butonic already invested days in finally funding a running solution for our ci system - this is enough from my pov at the moment.

@icewind1991
Copy link
Contributor

The unit tests crash on a btrfs docker host ...

Run sudo modprobe loop beforehand

@MorrisJobke
Copy link
Contributor

Run sudo modprobe loop beforehand

Already solved: #19864

@butonic
Copy link
Member Author

butonic commented Oct 19, 2015

👍 Awesome work icewind! Thanks for taking a look at this!

butonic added a commit that referenced this pull request Oct 19, 2015
don't move files in cache twice, fixes renaming for objectstores
@butonic butonic merged commit 2895c91 into master Oct 19, 2015
@butonic butonic deleted the fix_objectstore_rename branch October 19, 2015 15:19
@MorrisJobke
Copy link
Contributor

@butonic can you create the backport? Thanks

@butonic
Copy link
Member Author

butonic commented Oct 20, 2015

will create PRs for 8.2 and then try 8.1

@DeepDiver1975
Copy link
Member

will create PRs for 8.2 and then try 8.1

up to now we did only talk about 8.2 - what is the need to have this in 8.1?

@butonic
Copy link
Member Author

butonic commented Oct 20, 2015

probably none with the next enterprise edition being based on 8.2. Let's see who starts screaming first...

@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants